Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor zabbix web #1235

Merged
merged 23 commits into from
Jun 11, 2024
Merged

Conversation

eb4x
Copy link
Collaborator

@eb4x eb4x commented May 25, 2024

SUMMARY

This is a refactor of the zabbix_web role.

ISSUE TYPE
  • Refactor Pull Request
COMPONENT NAME

zabbix_web role

ADDITIONAL INFORMATION

Some small fixes to zabbix_server role, zabbix_web uses a more recent version of mysql, which uncovered some issues with pymysql.

@eb4x
Copy link
Collaborator Author

eb4x commented May 25, 2024

I'm posting this as a draft, I haven't figured it all out yet. There are some things that don't really line up between the os_families. Like why are php packages installed for debian, but redhat packages goes unused.

@eb4x eb4x force-pushed the refactor_zabbix_web branch from 3621324 to 6e75aa3 Compare May 30, 2024 23:19
@eb4x eb4x mentioned this pull request May 30, 2024
@eb4x eb4x force-pushed the refactor_zabbix_web branch from 6e75aa3 to a02ac1a Compare June 5, 2024 10:13
@eb4x eb4x force-pushed the refactor_zabbix_web branch 2 times, most recently from 09cc9e0 to eab24ed Compare June 8, 2024 19:32
@eb4x eb4x marked this pull request as ready for review June 8, 2024 19:33
@eb4x eb4x changed the title WIP: Refactor zabbix web Refactor zabbix web Jun 8, 2024
@eb4x
Copy link
Collaborator Author

eb4x commented Jun 8, 2024

I'm taking this out of draft. I think the changes here are good cleanups. Not really done yet, but if merged, I'll create a new PR with the rest of the changes for this refactor. (otherwise I'll push to this when ready.)

eb4x added 15 commits June 8, 2024 21:51
RedHat can pin the minor version, and has a toggle to disable repo.
Debian can't pin minor version. The other debian option
cache_valid_time seems irrelevant?

We use the common package module, which is just a wrapper around
apt/yum, and use this construction;

  user_supplied_var | default(_calculated_var | default(omit))

to send additional parameters to the respective modules.

The workaround ZBX-10467 is dependent on packages being installed,
so had to get moved aswell.
It doesn't really make sense installing this package on all of
RedHat family, when there's no corresponding Debian task doing the
same. MySQL/mariadb is gets installed by the zabbix_server role.

And the PyMySQL dependency is only needed for the ansible collection
community.mysql, which we don't use in this role.
It's better to check for what it IS we're looking for rather than
what it's not. You could be looking at 'Suse' at some point, and
this logic wouldn't hold.
Use dnf module instead of command, this makes the task idempotent.
Remove ubuntu-22.04 from this workaround, support for ubuntu-22.04
was added in 5.0.0[1].

Not sure if it's a good idea to pin the geerlingguy role? We can
pin it to a specific version, but I haven't figured out why it's
not respecting a relative version, like '>=5.0.0'

[1] geerlingguy/ansible-role-php#406
Zabbix 5 wanted to install the sql-scripts to /usr/share/doc, and
this task removes a line in yum.conf that prevented that from
happening. But since 6.0 we haven't had to deal with that, so out
it goes.
This role doesn't support EL7 anymore, so we can remove these
tasks/checks.
This calculation doesn't change. So let's provide/reuse it across
the whole block.
PyMySQL 9>=,<10 did not have issues with this pinned mysql:8.0.32,
but does with later releases, so we bump pymysql for additional
debian and ubuntu releases.
This could have been an if-statement, I just don't like the
"looseness" of the else part, so I opted for a lookup-table.
Having them only applied with a when condition leaves no way to
undo the booleans once set. So we apply them regardless. If the
booleans are defaulting to false, we set them false on the system.
Use the generic `package` module for package installs, and install
this ansible dependency regardless.
@eb4x eb4x force-pushed the refactor_zabbix_web branch from eab24ed to a86fb55 Compare June 8, 2024 19:52
eb4x added 2 commits June 9, 2024 10:23
Also, this zabbix_agent_disable_repo is probably a copy-paste error
introduced at some point.
@eb4x eb4x force-pushed the refactor_zabbix_web branch from 7ee5107 to 3e1036b Compare June 9, 2024 08:59
By converting zabbix_web_http_server_package to a list of one
package, we can add zabbix_web_php_dependencies to it during
installation of those packages.
eb4x added 2 commits June 9, 2024 18:56
There is a corresponding php-mysql package, and we might aswell
make sure that is installed based on zabbix_server_database.
I'm not sure what this when condition is for;

  when:
    - zabbix_web_version is version('6.0', '!=')
    - ansible_distribution_major_version == '9'

But we're installing the package anyway. So just drop this task,
maybe?
@eb4x eb4x force-pushed the refactor_zabbix_web branch from cfde3c3 to 780c9c7 Compare June 9, 2024 16:57
@pyrodie18
Copy link
Collaborator

You good for me to pull this in @eb4x

@eb4x
Copy link
Collaborator Author

eb4x commented Jun 9, 2024

Yep just merge if you feel like. I can create a second PR once I figure out how to strip the last bits in tasks/RedHat.yml. I think these are all good parts of the refactor.

eb4x and others added 2 commits June 10, 2024 11:43
We don't need restart apache/nginx when a php file gets updated,
they are completely separate from running configuration.

We can also drop the >= zabbix-5.0 condition.

In the rest of the role, we're only calling the respective handler
when we need to, so no more when conditions needed.
Copy link
Collaborator

@pyrodie18 pyrodie18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK tracking that Proxy is currently failing but pretty sure it has nothing to do with this, and I'm already working on it. So going to pull this in so that #1194 can rebase and come in.

@pyrodie18 pyrodie18 merged commit 78658d6 into ansible-collections:main Jun 11, 2024
167 of 182 checks passed
pyrodie18 added a commit to pyrodie18/community.zabbix that referenced this pull request Jun 12, 2024
* Single common task to install zabbix-web

RedHat can pin the minor version, and has a toggle to disable repo.
Debian can't pin minor version. The other debian option
cache_valid_time seems irrelevant?

We use the common package module, which is just a wrapper around
apt/yum, and use this construction;

  user_supplied_var | default(_calculated_var | default(omit))

to send additional parameters to the respective modules.

The workaround ZBX-10467 is dependent on packages being installed,
so had to get moved aswell.

* Remove mysql install tasks

It doesn't really make sense installing this package on all of
RedHat family, when there's no corresponding Debian task doing the
same. MySQL/mariadb is gets installed by the zabbix_server role.

And the PyMySQL dependency is only needed for the ansible collection
community.mysql, which we don't use in this role.

* Improve when condition logic for os_family

It's better to check for what it IS we're looking for rather than
what it's not. You could be looking at 'Suse' at some point, and
this logic wouldn't hold.

* Swap out remi for appstream php:8.0/common

Use dnf module instead of command, this makes the task idempotent.

* Make it clear tasks are a workaround

Remove ubuntu-22.04 from this workaround, support for ubuntu-22.04
was added in 5.0.0[1].

Not sure if it's a good idea to pin the geerlingguy role? We can
pin it to a specific version, but I haven't figured out why it's
not respecting a relative version, like '>=5.0.0'

[1] geerlingguy/ansible-role-php#406

* Remove zabbix 5.0 related task

Zabbix 5 wanted to install the sql-scripts to /usr/share/doc, and
this task removes a line in yum.conf that prevented that from
happening. But since 6.0 we haven't had to deal with that, so out
it goes.

* Remove tasks and checks guarding against EL7

This role doesn't support EL7 anymore, so we can remove these
tasks/checks.

* Move delegated_dbhost to first block

This calculation doesn't change. So let's provide/reuse it across
the whole block.

* Bump mysql container image, add workarounds

PyMySQL 9>=,<10 did not have issues with this pinned mysql:8.0.32,
but does with later releases, so we bump pymysql for additional
debian and ubuntu releases.

* Increase similarity in init-mysql between roles

* Provide the default port for database servers

This could have been an if-statement, I just don't like the
"looseness" of the else part, so I opted for a lookup-table.

* Remove unused zabbix_selinux_dependencies

This lookup goes unused.

* Group selinux booleans, and apply regardless

Having them only applied with a when condition leaves no way to
undo the booleans once set. So we apply them regardless. If the
booleans are defaulting to false, we set them false on the system.

* Install selinux packages regardless

Use the generic `package` module for package installs, and install
this ansible dependency regardless.

* RedHat: zabbix_web_php_dependencies goes unused

* Single task for installing zabbix-apache-conf

Also, this zabbix_agent_disable_repo is probably a copy-paste error
introduced at some point.

* Single task for installing zabbix-nginx-conf

* Move installation of debian php deps

By converting zabbix_web_http_server_package to a list of one
package, we can add zabbix_web_php_dependencies to it during
installation of those packages.

* Move installation of debian php-pgsql

There is a corresponding php-mysql package, and we might aswell
make sure that is installed based on zabbix_server_database.

* RedHat: Remove zabbix-{{ ..._http_server }}-conf

I'm not sure what this when condition is for;

  when:
    - zabbix_web_version is version('6.0', '!=')
    - ansible_distribution_major_version == '9'

But we're installing the package anyway. So just drop this task,
maybe?

* zabbix_underscore_version goes unused

* Remove useless when conditions and notifies

We don't need restart apache/nginx when a php file gets updated,
they are completely separate from running configuration.

We can also drop the >= zabbix-5.0 condition.

In the rest of the role, we're only calling the respective handler
when we need to, so no more when conditions needed.

---------

Co-authored-by: Troy W <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants